Improve error UX when chat proposal parsing fails#582
Conversation
…atching Replace flat error string in AutomationPlannerService with a structured ParseHintPayload containing supportedPatterns array, detectedIntent, closestPattern, and exampleInstruction. Uses keyword scoring to find the most relevant pattern suggestion for the user's input.
Detect structured parse hint marker in planner error messages and set messageType to parse-hint so the frontend can render a hint card instead of inline error text. Non-hint errors still fall through to status type.
Display parse-hint messages as a styled info card showing detected intent, closest matching pattern, and a pre-fill button. Include a collapsible list of all supported patterns. Card uses info styling, not error styling.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Self-Review FindingsAPI contract
XSS risk
Hardcoded strings
Accessibility
Edge cases
State management
Potential concern
Test coverage
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a structured 'parse hint' system to assist users when automation instructions cannot be parsed. The backend now identifies user intent and suggests the closest matching command pattern, returning this data as a JSON payload. The frontend has been updated to render these hints as interactive cards, allowing users to quickly apply suggested formats. Feedback focuses on refactoring the intent detection logic for better maintainability and optimizing the frontend rendering to avoid redundant parsing of the hint data.
I am having trouble creating individual review comments. Click here to see my feedback.
backend/src/Taskdeck.Application/Services/AutomationPlannerService.cs (461-479)
The DetectIntent method uses a series of if statements which can become hard to maintain as more intents are added. Consider refactoring this to use a switch expression for a more modern and declarative approach that is easier to read and extend.
internal static string? DetectIntent(string instruction)
{
var lower = instruction.Trim().ToLowerInvariant();
return lower switch
{
var s when s.Contains("create") || s.Contains("add") || s.Contains("new") => "create",
var s when s.Contains("move") || s.Contains("drag") || s.Contains("transfer") => "move",
var s when s.Contains("archive") || s.Contains("remove") || s.Contains("delete") => "archive",
var s when s.Contains("update") || s.Contains("edit") || s.Contains("change") || s.Contains("rename") || s.Contains("set") => "update",
var s when s.Contains("unarchive") || s.Contains("restore") => "unarchive",
var s when s.Contains("reorder") || s.Contains("position") => "reorder",
_ => null
};
}frontend/taskdeck-web/src/views/AutomationChatView.vue (604-651)
The getParseHint(message) function is called multiple times within the template for each message. This is inefficient as it re-parses the message content on every render cycle. This also forces the use of the non-null assertion operator (!), which can be brittle.
A better approach is to process the messages once. You can create a computed property that memoizes the parsed hint for each message.
In <script setup>:
const messagesWithHints = computed(() =>
sortedMessages.value.map(message => ({
...message,
parsedHint: getParseHint(message),
}))
);In <template>:
Then, iterate over messagesWithHints and use message.parsedHint directly, which will be either the parsed object or null. This avoids repeated parsing and the need for !.
For example:
<div v-for="message in messagesWithHints" ...>
...
<template v-if="message.messageType === 'parse-hint' && message.parsedHint">
<div
class="td-message-content td-message-content--markdown"
v-html="renderMarkdown(message.parsedHint.textBeforeHint)"
></div>
<div class="td-hint-card" ...>
...
{{ message.parsedHint.hint.detectedIntent ? ... }}
...
</div>
</template>
...
</div>
Adversarial Review -- PR #582CriticalNone found. Major1. The The existing test only checks "restore the board" for the unarchive intent, which happens to work because "restore" does not contain "archive". A test for Fix: Move the 2. Substring matching in if (words.Any(w => w.Contains(keyword)))This checks if any word in the instruction contains the keyword as a substring. Short keywords like
This inflates scores for wrong patterns. Should use exact word equality ( Minor3. If the LLM echoes a user message containing the literal text Mitigation: use 4. No test for The test at line 862 only tests 5. If the user is typing in the message textarea and clicks "Try this instead", their in-progress text is silently replaced. There is no confirmation dialog or undo mechanism. This is a minor UX concern -- the pre-fill should either warn or only apply when the textarea is empty. Nits6. The self-review and Gemini both flagged repeated 7. The keyword 8. CSS
Overall AssessmentPass with fixes. The unarchive/archive ordering bug (#1) and substring keyword matching (#2) are correctness issues that will cause wrong suggestions in real usage. Both are straightforward fixes. The remaining items are robustness and polish concerns that can be addressed in follow-up. |
- Reorder DetectIntent checks so "unarchive" is checked before "archive" (previously "unarchive" was always misclassified as "archive" because the string contains the substring) - Switch from substring matching to whole-word matching in both DetectIntent and FindClosestPattern to prevent false positives (e.g. "sunset" matching "set", "address" matching "add") - Use lastIndexOf for PARSE_HINT marker in frontend to handle edge case where LLM response contains the marker text - Add regression tests for "unarchive my board" and "rename board"
Follow-up: Fixes pushed (37f3f49)Addressed Major issues #1 and #2 from the adversarial review, plus Minor #3: 1. DetectIntent ordering bug (Major #1)
2. Substring matching in FindClosestPattern (Major #2)
3. PARSE_HINT marker robustness (Minor #3)
Verification:
|
Update two analysis docs (chat-to-proposal gap and manual testing findings) to reflect recent fixes and testing status. Key changes: add Last Updated and status notes; mark Tier 1 improvements shipped (intent classifier regex/stemming/negation fixes, substring ordering bug, PR #579), UX parse hints shipped (PR #582), unit/integration tests shipped (PR #580), and note PR range #578–#582. In manual testing findings mark OBS-2/OBS-3 resolved (PR #581) and BUG-M5 resolved (PR #578), update resolutions and remove duplicate checklist items. Minor editorial clarifications and test counts added.
1 participant
Summary
ParseHintPayloadcontainingsupportedPatternsarray,detectedIntent,closestPattern, andexampleInstruction. Uses keyword scoring to suggest the most relevant pattern.messageTypetoparse-hintfor frontend rendering. Non-hint errors still use the existingstatustype (backward compatible).parse-hintmessages as a styled info card (not error-style) with detected intent display, closest pattern suggestion, "Try this instead" button that pre-fills the chat input, and a collapsible list of all supported patterns.Test plan